feat: restructure repository#40
feat: restructure repository#40akihikokuroda wants to merge 37 commits intogenerative-computing:mainfrom
Conversation
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Add detect-changes job that checks which subpackages have been modified and outputs flags for each. Each subpackage workflow now only runs when: - Its files in mellea_contribs/ are changed, OR - Its corresponding quality workflow file is changed This prevents unnecessary CI runs when only unrelated subpackages are modified. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
jakelorocco
left a comment
There was a problem hiding this comment.
Is it possible to make the workflows dynamic? Ie instead of having to create a new one for each subpackage, we just mandate all folders under mellea_contribs are subpackages and require some sort of structure?
Even if we don't have that, we should make sure to have a version of the quality check that ensures new packages have tests. For example, if I make a new subpackage and don't add tests / quality workflows right now, will I be able to merge? Or there's some necessary check still?
I'm also not certain how these differently named checks interact with github branch protections? For the mellea repo, we have to specifically select checks.
|
@jakelorocco A new CI that checks if each subpackage has its own ci can be added. The quality of each ci is up-to the subpackage though. Is it enough as a minimum? |
I think that makes sense. We should vet CI at merge time, but a check will ensure nothing gets through accidentally. |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
Once this merges, we'll also need to update the required checks in the branch protections ( as the quality checks no longer exist) |
.github/workflows/ci.yml
Outdated
| on: | ||
| pull_request: | ||
| types: [opened, reopened, synchronize] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
this would need to be run manually right? would this be more for auditing or do you see another purpose?
There was a problem hiding this comment.
Yes, this must be run manually. I just left it for convenience. It can be deleted.
There was a problem hiding this comment.
So we can't have checks on this repo then? Or the checks just need to be manually kicked off for a given branch / PR?
There was a problem hiding this comment.
I also don't know how branch protections interact with this. And also, if this is the case, why are the github action runners showing that checks ran below in the PR?
| push: | ||
| branches: | ||
| - main | ||
| paths: |
There was a problem hiding this comment.
since all the backends depend on mellea-integration-core, there is the chance that if we change something in the core that it would break the backends... should we also trigger a run of the backends if the core changes?
There was a problem hiding this comment.
That's right. I'll add mellea-integration-core change in all integration paths.
There was a problem hiding this comment.
I don't think we should create cross-dependencies in this repo. See my comment below, but if we update mellea-integration-core and it forces us to update other packages, then we've just inherited a ton of maintenance work.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
|
||
| dependencies = [ | ||
| "mellea>=0.3.2", | ||
| "mellea==0.3.2", |
There was a problem hiding this comment.
do we want to bump this to 0.4.0?
longer-term, we probably need to sync our update strategy here to the release process
cc @jakelorocco
There was a problem hiding this comment.
I only flagged one, but there are multiple places we may need to update if we do want to bump
There was a problem hiding this comment.
Some tests failed with 0.4.0. Bumping 0.4.0 will be follow up PR.
There was a problem hiding this comment.
I think it's fine for these to lag behind mellea (and expected since we don't want to own all the maintenance here). What happens if we bump mellea-integration-core? Do the internal packages that rely on it in here also have to get bumped? If so, that seems untenable.
psschwei
left a comment
There was a problem hiding this comment.
I think I am largely good with the changes here. would be good to have a review from the core team as well
jakelorocco
left a comment
There was a problem hiding this comment.
I think this is looking good and on the right track. The main purpose of my comments is to make longterm support from the Mellea team much less burdensome in this repo. To me, that means automating a large amount of the testing process and ensuring subpackages can be updated independently when needed.
run_tests.sh
Outdated
| # Test reqlib_package (may have dependency issues) | ||
| echo "==========================================" | ||
| echo "Testing: reqlib_package" | ||
| echo "==========================================" | ||
| echo -e "${YELLOW}Note: reqlib_package requires additional dependencies (citeurl, eyecite, playwright)${NC}" | ||
| cd "$REPO_ROOT/mellea_contribs/reqlib_package" | ||
| if PYTHONPATH="src:$PYTHONPATH" python3 -m pytest test/ -v --tb=short 2>&1; then | ||
| echo -e "${GREEN}✓ reqlib_package tests PASSED${NC}" | ||
| PASSED_PROJECTS+=("reqlib_package") | ||
| else | ||
| echo -e "${YELLOW}⚠ reqlib_package tests SKIPPED (missing dependencies)${NC}" | ||
| FAILED_PROJECTS+=("reqlib_package (missing deps)") | ||
| fi | ||
| echo "" | ||
|
|
||
| # Test tools_package (may have dependency issues) | ||
| echo "==========================================" | ||
| echo "Testing: tools_package" | ||
| echo "==========================================" | ||
| echo -e "${YELLOW}Note: test_mprogram_robustness.py requires 'benchdrift' package (skipped)${NC}" | ||
| cd "$REPO_ROOT/mellea_contribs/tools_package" | ||
| if PYTHONPATH="src:$PYTHONPATH" python3 -m pytest test/test_double_round_robin.py test/test_top_k.py -v --tb=short 2>&1; then | ||
| echo -e "${GREEN}✓ tools_package tests PASSED (2 tests, test_mprogram_robustness.py skipped - requires benchdrift)${NC}" | ||
| PASSED_PROJECTS+=("tools_package") | ||
| else | ||
| echo -e "${YELLOW}⚠ tools_package tests FAILED${NC}" | ||
| FAILED_PROJECTS+=("tools_package") | ||
| fi | ||
| echo "" |
There was a problem hiding this comment.
We should mandate that all projects have the same structure. We shouldn't need to specify different commands here than from the run_project_tests above.
|
|
||
| dependencies = [ | ||
| "mellea>=0.3.2", | ||
| "mellea==0.3.2", |
There was a problem hiding this comment.
I think it's fine for these to lag behind mellea (and expected since we don't want to own all the maintenance here). What happens if we bump mellea-integration-core? Do the internal packages that rely on it in here also have to get bumped? If so, that seems untenable.
| push: | ||
| branches: | ||
| - main | ||
| paths: |
There was a problem hiding this comment.
I don't think we should create cross-dependencies in this repo. See my comment below, but if we update mellea-integration-core and it forces us to update other packages, then we've just inherited a ton of maintenance work.
| @@ -0,0 +1 @@ | |||
| README.md | |||
There was a problem hiding this comment.
I think we should either remove the README.md or add something here (even if it's auto generated). There's another README.md with the same problem.
.github/workflows/ci.yml
Outdated
There was a problem hiding this comment.
I might have missed it, but does this introduce the auto-detect packages without testing that we talked about?
.github/workflows/ci.yml
Outdated
| detect-changes: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| mellea-integration-core: ${{ steps.changes.outputs.mellea-integration-core }} | ||
| crewai-backend: ${{ steps.changes.outputs.crewai-backend }} | ||
| dspy-backend: ${{ steps.changes.outputs.dspy-backend }} | ||
| langchain-backend: ${{ steps.changes.outputs.langchain-backend }} | ||
| reqlib-package: ${{ steps.changes.outputs.reqlib-package }} | ||
| tools-package: ${{ steps.changes.outputs.tools-package }} |
There was a problem hiding this comment.
I feel like instead of having to manually add a bunch of boilerplate for any new package that gets added to this repo, we should enforce a required package structure. Then, we don't need to duplicate this code and can just auto-process any new additions and automatically handle new packages in CI runs.
.github/workflows/ci.yml
Outdated
| on: | ||
| pull_request: | ||
| types: [opened, reopened, synchronize] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
So we can't have checks on this repo then? Or the checks just need to be manually kicked off for a given branch / PR?
|
@jakelorocco I'm taking out ci.yml, README.md and run_test.sh. They are not necessary and may be causing issues later. I also take back the changes mellea_integration_core kicking the 3 integrations ci. |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

fix #38